Integrate admin bar with AMP dev mode #3187

westonruter · 2019-09-05T00:45:18Z

See full writeup: Integrating with AMP Dev Mode in WordPress.

Skip sanitizing admin bar (and removing it) in favor of ignoring (in dev mode).
Introduce amp_is_dev_mode() function along with an amp_dev_mode_enabled filter; this returns true by default when the user is logged-in and the admin bar is showing.
Introduce an AMP_Dev_Mode_Sanitizer filter which is registered among the sanitizers when amp_is_dev_mode() returns true. Takes as an argument an array of XPath expressions for elements to add data-ampdevmode to; this array can be filtered via the amp_dev_mode_element_xpaths hook. When the admin bar is shown, it will at least include the XPath expressions for excluding admin bar.
Eliminate need for keeping forked version of admin-bar.css in plugin.
Automatically add data-ampdevmode attributes to scripts and styles that depend on the admin bar, excluding them from being sanitized out of the document. This includes Dashicons, when the stylesheet is not being enqueued as part of the admin bar.
Skip processing stylesheets and converting elements that have data-ampdevmode attributes.

Fixes #1921.

Reverts or partially-undoes #2346, #2972, #2500, #2405.

Integrates with Dev Mode introduced in ampproject/amphtml#20974 via #3084.

Before

When themes have too much CSS for the admin bar to fit, the admin bar was removed from the page:

When there was not too much CSS, the admin bar was included in post-processing, converting all elements in the admin bar to AMP (including the Gravatar to amp-img) and processing/tree-shaking the admin-bar.css and dashicons.css; the admin-bar.js was dequeued and any plugins that tried to add JS to put interactivity in the admin bar were blocked as validation errors:

After

The admin bar element tree, the admin bar stylesheets (admin-bar.css, its inline style, and dashicons.css if not directly enqueued by a theme/plugin) will all be marked with the dev-ampdevmode attribute, in addition to this the data-ampdevmode attribute being added to the root element. The AMP validator extension will show one single error related to dev mode, but the extension will soon be updated to change this from an error badge to some other indicator specific dev mode:

The root HTML element, the admin bar element tree, the styles for the admin bar (admin-bar.css, inline style, dashicons.css), and now even the admin-bar.js all get the data-ampdevmode attribute added and thus are excluded from sanitization:

Now plugins can exempt JS administrative functionality in the admin bar from AMP validation. For example, with Automattic/jetpack#13450 the Notifications module in Jetpack now works on AMP pages:

The required scripts and styles all get the data-ampdevmode attribute:

Even when there is excessive CSS on the page, the admin bar will still be included:

Todo

Build for testing: amp.zip - v1.3-alpha-20190910T210727Z-e15bbe33

This reverts commit d83bb25 from #2500.

…dev mode) * Eliminate need for keeping forked version of admin-bar.css in plugin. * Automatically add data-ampdevmode attributes to scripts and styles that depend on the admin bar, excluding them from being sanitized out of the document. * Skip processing stylesheets that have data-ampdevmode attributes.

Also add missing dynamic_element_selectors arg to stylesheet cache key.

…s for dev mode elements

…y for unauthenticated admin bars

Refactor methods to improve readability and organization

Add todo comments for where dev mode should be used instead of allow_dirty_*. Make closures static.

westonruter · 2019-09-07T18:18:42Z

I'm going to add some more tests, but the scope of this I believe is complete.

westonruter · 2019-09-08T00:59:02Z

I'm also going to open PRs for Jetpack and Site Kit to ensure the API allows for the plugins to successfully exempt admin bar integrations from AMP validation.

includes/class-amp-theme-support.php

schlessera

Some typos in comments and a few optional remarks from my side.

schlessera · 2019-09-09T11:43:03Z

includes/class-amp-story-post-type.php

@@ -233,6 +233,17 @@ function() {
 		add_action( 'amp_story_head', 'wp_site_icon', 99 );
 		add_action( 'amp_story_head', 'wp_oembed_add_discovery_links' );

+		// Disable admin bar from even trying to be output, since wp_head and wp_footer hooks are not on the template.


Would it make sense (in a later PR) to hook the admin bar into 'amp_story_head'/'amp_story_footer' ?

That's a good question. I'm not sure if the styles used in AMP Stories would even be compatible with the styles used for the admin bar. Since AMP Stories are a full-screen experience, I suspect that the admin bar would clash. But it would be something useful to investigate at a later time, yes.

includes/class-amp-theme-support.php

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

…ode-support * 'develop' of github.com:ampproject/amp-wp: (28 commits) Exclude development files from production build ZIPs Add filter to allow more video types in AMP story background se… (#3171) Update dependency @babel/cli to v7.6.0 (#3203) Update dependency @babel/core to v7.6.0 (#3204) Remove deprecated AMP_WP_Utils class Refresh Composer lock file Run build in a temporary folder Add shell script to clean up the current folder and then trigger a build Adapt Gruntfile to fetch optimized Composer dependencies within the build folder Retrieve Sabberworm patch directly from the GitHub PR Adding regression test for vertical text alignment issues Fixing minor errors in test descriptions Only restrict height inside non-fit text blocks Rewritten block size tests Limit height declaration to only direct descendant Remove now obsolete withEnforcedVideoUploadType HOC Update eslint-plugin-jest and apply fixes (#3192) Update dependency postcss to v7.0.18 (#3200) Update dependency terser-webpack-plugin to v2 (#3195) Update dependency webpack-cli to v3.3.8 (#3194) ...

schlessera · 2019-09-10T07:29:25Z

includes/amp-helper-functions.php

@@ -899,6 +935,33 @@ function amp_get_content_sanitizers( $post = null ) {
 	 */
 	$sanitizers = apply_filters( 'amp_content_sanitizers', $sanitizers, $post );

+	if ( amp_is_dev_mode() ) {


I think it would be preferable to have this piece of logic be part of the AMP_Dev_Mode_Sanitizer class instead, and then make that sanitizer decide when it acts or not, instead of hardcoding it in here.

It should be the responsibility of the AMP_Dev_Mode_Sanitizer to know when to do something and what to do, not some helper function collection.

I did think about that, but I think it's preferable here because this function is inherently connected directly with WordPress as it applies a filter. There are two reasons for why this should not be in the sanitizer:

It is important that the array of sanitizer args returned by amp_get_content_sanitizers() be deterministic in what the resulting sanitized document be. In other words, we cannot apply filters inside of a sanitizer because then we cannot use the sanitizer args as a source of truth for computing a cache key for the post-processed output, as we can now.

We should be trending the sanitizers toward being pure PHP classes that are do not connect with WordPress APIs (especially the plugin API per above) as this is the direction needed for abstracting the sanitizers onto a CMS-agnostic package.

Additionally, this amp_is_dev_mode() function would be useful outside of the context of a sanitizer. For example, a theme/plugin may want to call this function during template generation to determine whether it can output some dev code (e.g. Query Monitor).

The amp_is_dev_mode() is gatekeeping whether or not the AMP_Dev_Mode_Sanitizer should be used in the first place. So it is operating at a higher level.

Aside: should it rather be named is_amp_dev_mode()?

Aside: should it rather be named is_amp_dev_mode()?

I'm seeing the amp_ as a prefix here, as this could just as well be used outside of the AMP plugin, so I think the name is fine.

tests/php/test-amp-helper-functions.php

westonruter · 2019-09-10T14:27:41Z

Just found an issue. When the theme has a lot of CSS and the admin bar is showing up, the admin-bar.css could be included but dashicons.css could get excluded, resulting in:

We should probably mark the link element for loading Dashicons with data-ampdevmode, though there is a chance that a theme/plugin is enqueueing it for use on the frontend for use on pages even without the admin bar.

westonruter · 2019-09-10T14:30:53Z

What comes to mind is that we could mark Dashicons as being in dev mode only if it was not directly enqueued or a dependency of any enqueued stylesheet other than admin-bar.

…min bar

includes/class-amp-theme-support.php

tests/php/test-class-amp-theme-support.php

…cons

googlebot · 2019-09-11T15:34:10Z

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

schlessera · 2019-09-11T15:35:07Z

@googlebot I consent.

googlebot · 2019-09-11T15:35:19Z

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

… requests"" This reverts commit 3380451 from #3187.

westonruter added 2 commits September 4, 2019 16:49

Revert "Suppress admin bar from being shown during validation requests"

3380451

This reverts commit d83bb25 from #2500.

westonruter added this to the v1.3 milestone Sep 5, 2019

googlebot added the cla: yes Signed the Google CLA label Sep 5, 2019

westonruter added 6 commits September 5, 2019 17:44

Prevent tree-shaking under #wpadminbar when admin bar is showing

8640d1a

Also add missing dynamic_element_selectors arg to stylesheet cache key.

Prevent admin bar from even attempting to be displayed on story template

f75586f

Remove redundant document argument from is_document_in_dev_mode()

a81fb73

Skip converting elements in dev mode; skip processing style attribute…

bfad786

…s for dev mode elements

Add filters for whether dev mode is enabled and what elements get it

5bb7466

Bring back remove_admin_bar_if_css_excluded and admin-bar CSS priorit…

d615d39

…y for unauthenticated admin bars

westonruter changed the title ~~Integrate admin bar and Customizer with AMP dev mode~~ Integrate admin bar with AMP dev mode Sep 7, 2019

westonruter added 2 commits September 7, 2019 11:01

Prevent dev mode when admin bar shown for unauthenticated users

277131f

Refactor methods to improve readability and organization

Fix placement of dev_mode_enabled variable definition

4b533d2

Add todo comments for where dev mode should be used instead of allow_dirty_*. Make closures static.

westonruter requested a review from schlessera September 7, 2019 18:18

westonruter marked this pull request as ready for review September 7, 2019 18:18

Fix undefined dev_mode_xpaths being used in cache key

e3025cd

This was referenced Sep 8, 2019

Add support for AMP dev mode google/site-kit-wp#505

Merged

Use inline style system to add admin bar styles to page pantheon-systems/pantheon-hud#44

Merged

Add support for AMP Dev Mode in Notes module Automattic/jetpack#13450

Merged

westonruter commented Sep 9, 2019

View reviewed changes

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved

schlessera requested changes Sep 9, 2019

View reviewed changes

westonruter and others added 6 commits September 9, 2019 10:00

Fix typos in comments and add array casting

a726d41

Co-Authored-By: Alain Schlesser <alain.schlesser@gmail.com>

Move dev mode mutations into new sanitizer

d1ac164

Remove now-unnecessary admin_bar_showing arg for style sanitizer

5ba10b4

Add tests for init_admin_bar

38fa7ba

Add test for amp_is_dev_mode()

71c195c

schlessera requested changes Sep 10, 2019

View reviewed changes

westonruter added 2 commits September 10, 2019 13:56

Include dashicons in dev mode but only if exclusively connected to ad…

e15bbe3

…min bar

Remove amp_dev_mode_enabled filter in test

d79be8d

westonruter requested a review from schlessera September 10, 2019 21:22

westonruter mentioned this pull request Sep 10, 2019

Ignore admin bar for AMP validation purposes #1921

Closed

5 tasks

schlessera requested changes Sep 11, 2019

View reviewed changes

includes/class-amp-theme-support.php Outdated Show resolved Hide resolved

tests/php/test-class-amp-theme-support.php Show resolved Hide resolved

tests/php/test-class-amp-theme-support.php Show resolved Hide resolved

schlessera added 2 commits September 11, 2019 17:28

Remove useless variable

5de22ff

Add asserts to make sure we are not enqueueing both versions of dashi…

290adf7

…cons

googlebot added cla: no Has not signed the Google CLA and removed cla: yes Signed the Google CLA labels Sep 11, 2019

googlebot added cla: yes Signed the Google CLA and removed cla: no Has not signed the Google CLA labels Sep 11, 2019

schlessera approved these changes Sep 11, 2019

View reviewed changes

westonruter merged commit dd5a130 into develop Sep 11, 2019

westonruter deleted the add/dev-mode-support branch September 11, 2019 16:16

westonruter added the Has Screencast label Sep 11, 2019

westonruter mentioned this pull request Sep 12, 2019

Style sanitizer contains a piece of logic that doesn't seem to have any effect #3232

Closed

schlessera mentioned this pull request Sep 13, 2019

Remove unused piece of code in Style Sanitizer #3235

Merged

This was referenced Sep 19, 2019

CSS missing only when logged in #3299

Closed

Include yoast-seo-adminbar style in AMP dev mode Yoast/wordpress-seo#13502

Merged

westonruter mentioned this pull request Sep 27, 2019

Add paired browsing interface for Transitional mode to aid with checking parity #3365

Closed

westonruter mentioned this pull request Nov 8, 2019

Remove redundant data-ampdevmode attributes from admin bar descendants #3701

Closed

3 tasks

westonruter added a commit that referenced this pull request Jan 28, 2020

Revert "Revert "Suppress admin bar from being shown during validation…

bb4e6d9

… requests"" This reverts commit 3380451 from #3187.

westonruter mentioned this pull request Jan 29, 2020

Fix validation error reporting for invalid attributes on the HTML element #4190

Closed

6 tasks

westonruter mentioned this pull request Mar 14, 2020

Validation errors not reported for invalid attributes on the HTML element #4387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate admin bar with AMP dev mode #3187

Integrate admin bar with AMP dev mode #3187

westonruter commented Sep 5, 2019 •

edited

Loading

westonruter commented Sep 7, 2019

westonruter commented Sep 8, 2019

schlessera left a comment

schlessera Sep 9, 2019

westonruter Sep 9, 2019

schlessera Sep 10, 2019

westonruter Sep 10, 2019

schlessera Sep 10, 2019

westonruter commented Sep 10, 2019

westonruter commented Sep 10, 2019

googlebot commented Sep 11, 2019

schlessera commented Sep 11, 2019

googlebot commented Sep 11, 2019

Integrate admin bar with AMP dev mode #3187

Integrate admin bar with AMP dev mode #3187

Conversation

westonruter commented Sep 5, 2019 • edited Loading

Before

After

Todo

westonruter commented Sep 7, 2019

westonruter commented Sep 8, 2019

schlessera left a comment

Choose a reason for hiding this comment

schlessera Sep 9, 2019

Choose a reason for hiding this comment

westonruter Sep 9, 2019

Choose a reason for hiding this comment

schlessera Sep 10, 2019

Choose a reason for hiding this comment

westonruter Sep 10, 2019

Choose a reason for hiding this comment

schlessera Sep 10, 2019

Choose a reason for hiding this comment

westonruter commented Sep 10, 2019

westonruter commented Sep 10, 2019

googlebot commented Sep 11, 2019

schlessera commented Sep 11, 2019

googlebot commented Sep 11, 2019

westonruter commented Sep 5, 2019 •

edited

Loading